Skip to content

Feature - DateTime::createFromImmutable() method #1145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 23, 2015

Conversation

Rican7
Copy link
Contributor

@Rican7 Rican7 commented Mar 5, 2015

Introduction

The DateTimeImmutable class, added in version 5.5, was a very convenient addition to the bundled date/time extension. Unfortunately, when migrating legacy codebases or when using an application that needed to mix immutable/mutable instances, the DateTimeImmutable class fell short in convenience by not providing a clear/clean way to build an immutable instance from a mutable instance.

This problem was solved in version 5.6 with the addition of a convenient factory method: DateTimeImmutable::createFromMutable(). Unfortunately, however, this addition created a one-sided convenience where a similar problem has been unresolved: Its still just as difficult to create mutable DateTime instances from an immutable DateTimeImmutable instance.

Arguably, creating DateTime instances from the immutable representations is something that an application would actually do more often, as any current (or legacy) applications and libraries are more likely to have function/method API's that require DateTime instances as parameters. While ideally the parameter types of new libraries would be DateTimeInterface (or at least DateTimeImmutable), that option is only available to libraries and applications that support PHP 5.5 as a minimum version... which is currently not the majority.

Without this factory method available to the DateTime class, its quite cumbersome to translate back and forth between the types, and honestly is quite the deterrent to even using the DateTimeImmutable class because of this. For example, if I was using a new library or application that dealt with DateTimeImmutable objects, but still had to interact with current/older libraries, I'd need to do work like this quite often:

// Use some cool new library that returns a `DateTimeImmutable`
$date_time_immutable = $new_cool_library->getStartTime();

// Use an existing library that accepts a `DateTime` concrete type
$date_time = new DateTime(null, $date_time_immutable->getTimeZone());
$date_time->setTimestamp($date_time_immutable->getTimestamp());
$existing_event_library->setTriggerAt($date_time);

As you can imagine, doing the above for multiple calls around an application/library is quite cumbersome, and often leads to potentially buggy user-land implementations to keep the code "DRY" or even worse: simply deters a library/application author from using the new DateTimeImmutable object despite its wonderful advantages.

Proposal

This PR proposes to introduce a single new method on the DateTime class with the following signature:

public static DateTime DateTime::createFromImmutable (DateTimeImmutable $datetime)

The resulting object would contain the same date/time value as the provided immutable parameter, however modifications to the resultant DateTime object would only modify that instance and not the immutable instance that was given as an argument. In other words, this simply copies state. It would be quite similar to a type-casted clone (if that were possible).

This proposal was designed to mirror the currently accepted and in-use DateTimeImmutable::createFromMutable() method. It contains a similar signature and would compliment the aforementioned method well.

Backward Incompatible Changes

None. This is a new static method on the concrete DateTime class (not on the Interface), so no extending classes would have to worry about breaking here.

Proposed PHP Version(s)

PHP 7

Affected PHP Functionality

To SAPIs

None

To Existing Extensions

None

To Opcache

None

Rican7 added 4 commits March 5, 2015 01:58
@Rican7 Rican7 force-pushed the feature/datetime-immutable-to-mutable branch from d1b3006 to 1de1b6f Compare March 5, 2015 14:51
@ghost
Copy link

ghost commented Mar 7, 2015

Can one of the admins verify this patch?

@smalyshev
Copy link
Contributor

Looks ok. @jpauli, @Tyrael - what do you think about merging this into 5.5/5.6?

@@ -2832,6 +2837,34 @@ PHP_METHOD(DateTime, __wakeup)
}
/* }}} */

/* {{{ proto DateTime::createFromImmutable(DateTimeImmutable object)
Creates new DateTime object from an existing DateTimeImmutable object.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The */ should be on the previous line, at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rican7
Copy link
Contributor Author

Rican7 commented Mar 13, 2015

Travis is 💚

@php-pulls php-pulls merged commit 98c6567 into php:master Mar 23, 2015
@Rican7 Rican7 deleted the feature/datetime-immutable-to-mutable branch March 23, 2015 06:26
@Rican7
Copy link
Contributor Author

Rican7 commented Mar 23, 2015

Oh wow! Thank you so much! My first official PHP source-code contribution! 😊

@smalyshev
Copy link
Contributor

Reverted following ML discussion and Derick's objections here: http://markmail.org/message/3aesaaoqv6ihiw53#query:+page:1+mid:ukwizupev32ld5tg+state:results
Looks like this needs an RFC.

@smalyshev smalyshev added RFC and removed Feature labels Apr 1, 2015
@Kubo2
Copy link
Contributor

Kubo2 commented Apr 12, 2015

@smalyshev @php-pulls Shouldn't this one be reopened?
@Rican7 👍

@Majkl578
Copy link
Contributor

What has happened to this PR? I guess noone cared enough to make an RFC? :(
Absence of this method is another great incompatibility in PHP core...

The arguments in http://markmail.org/message/3aesaaoqv6ihiw53#query:+page:1+mid:ukwizupev32ld5tg+state:results do not make any sense. This method is not intended for emulating immutability in PHP <5.5.
How should one proceed when a library relies on DateTime only, for BC reasons? E.g. semver does not allow changing method signature from DateTime to DateTimeInterface.

So now, instead of clean:

DateTime::createFromImmutable($dateTime)

one has to write a nonsense like this:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

Is this really what you indirectly suggest, @derickr?

@Kubo2
Copy link
Contributor

Kubo2 commented Aug 27, 2015

+1

On 26 August 2015 at 18:44, Michael Moravec notifications@github.com
wrote:

What has happened to this PR? I guess noone cared enough to make an RFC? :(
Absence of this method is another great incompatibility in PHP core...

The arguments in
http://markmail.org/message/3aesaaoqv6ihiw53#query:+page:1+mid:ukwizupev32ld5tg+state:results
do not make any sense. This method is not intended for emulating
immutability in PHP <5.5.
How should one proceed when a library relies on DateTime only, for BC
reasons? E.g. semver does not allow changing method signature from DateTime
to DateTimeInterface.

So now, instead of clean:

DateTime::createFromImmutable($dateTime)

one has to write a nonsense like this:

DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())

Is this really what you indirectly suggest, @derickr
https://github.com/derickr?


Reply to this email directly or view it on GitHub
#1145 (comment).

@enumag
Copy link

enumag commented Feb 23, 2017

ping @derickr

Your reason is not exactly valid. I want to use DateTimeImmutable everywhere in my application. However there are some libraries then historically require DateTime instead of DateTimeInterface. I need to convert the DateTimeImmutable I work with normally to DateTime when calling them.

@Majkl578
Copy link
Contributor

Hi, I opened #2484 as a follow-up, because I still think this is a legitimate feature and the objections against did not make much sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants